fix: allow inherited intercepting routes to share target patterns#721
fix: allow inherited intercepting routes to share target patterns#721Debbl wants to merge 3 commits intocloudflare:mainfrom
Conversation
| const uniqueInterceptTargetPatterns = new Map<string, string>(); | ||
| for (const route of routes) { | ||
| for (const slot of route.parallelSlots) { | ||
| for (const intercept of slot.interceptingRoutes) { | ||
| // Inherited slots can surface the same intercepting page on multiple | ||
| // child routes. De-dupe by page path so we only validate distinct | ||
| // physical intercept definitions while still rejecting real conflicts. | ||
| uniqueInterceptTargetPatterns.set(intercept.pagePath, intercept.targetPattern); | ||
| } | ||
| } | ||
| } | ||
| validateRoutePatterns(Array.from(uniqueInterceptTargetPatterns.values())); |
There was a problem hiding this comment.
The old approach could have been wrapped in a set to achieve the same result I imagine.
I wonder if it could potentially be possible to have two conflicting pagePath values and therefore one overwrites the others targetPattern 🤔
There was a problem hiding this comment.
@james-elicx I changed it a bit. Mind taking another look?
There was a problem hiding this comment.
I think it could potentially be possible for two interceptions to target the same route pattern
What I meant when I wrote this was that you could potentially achieve the same desired outcome with:
[...new Set(routes.flatMap((route) =>
route.parallelSlots.flatMap((slot) =>
slot.interceptingRoutes.map((intercept) => intercept.targetPattern),
),
))]just deduping the array we originally created
what do you think?
There was a problem hiding this comment.
I updated the PR to follow your suggestion.
After discussing it a bit more with AI, I think the targetPattern dedupe is probably fine for this PR. What I still want to validate is the behavior when there are multiple intercepting routes targeting the same route, for example:
app/feed/page.tsx
app/feed/@modal/(..)photo/[id]/page.tsx
app/profile/page.tsx
app/profile/@modal/(..)photo/[id]/page.tsx
app/photo/[id]/page.tsx
I’m going to put together a small demo around this case to compare the behavior in Next.js and vinext, just to make sure we’re matching the intended semantics.
There was a problem hiding this comment.
No worries, I'll mark as draft for the moment while you're looking at that
|
/bigbonk review |
commit: |
There was a problem hiding this comment.
Good bug find and clean fix. The root cause analysis is correct — inherited slots cause the same physical intercepting page to appear on multiple discovered routes, and the old flatMap approach counted them as separate entries.
The fix is sound for the problem it solves. I have one concern about a remaining edge case and a few minor nits.
Remaining edge case (not a blocker for this PR): Two different physical intercepting pages (different pagePath) that happen to target the same pattern (e.g. app/a/@modal/(...)photo/[id]/page.tsx and app/b/@modal/(...)photo/[id]/page.tsx both targeting /photo/:id) would still produce duplicate values from Array.from(uniqueInterceptTargetPatterns.values()), which validateRoutePatterns would reject. In Next.js, multiple intercepting routes from different locations can intercept the same target — interception is scoped to the navigation context. But this is a pre-existing issue (the old flatMap code had the same behavior), so it's not a regression introduced here.
| } | ||
|
|
||
| export function collectInterceptTargetPatterns(routes: readonly AppRoute[]): string[] { | ||
| const uniqueInterceptTargetPatterns = new Map<string, string>(); |
There was a problem hiding this comment.
To address @james-elicx's review comment: the Map is keyed by pagePath (physical file) with targetPattern as the value. So two different physical intercepting pages that target the same pattern would both be stored (separate keys), and Array.from(values()) would emit that pattern twice — causing validateRoutePatterns to throw.
This isn't a regression (the old flatMap had the same problem), but worth noting that de-duplicating by targetPattern in the returned array would be more correct for the general case. The simplest approach would be to return values through a Set:
| const uniqueInterceptTargetPatterns = new Map<string, string>(); | |
| const uniqueInterceptTargetPatterns = new Map<string, string>(); |
Actually the Map type is fine as-is — but consider de-duping the return value at line 159:
return [...new Set(uniqueInterceptTargetPatterns.values())];
This would handle the case where two different physical pages legitimately intercept the same target pattern (e.g. via (...) from different subtrees). Not a blocker for this PR, but a follow-up improvement.
| } | ||
| } | ||
|
|
||
| return Array.from(uniqueInterceptTargetPatterns.values()); |
There was a problem hiding this comment.
Consider de-duplicating the returned values to handle the case where two different physical pages (different pagePath) legitimately intercept the same targetPattern:
| return Array.from(uniqueInterceptTargetPatterns.values()); | |
| return Array.from(new Set(uniqueInterceptTargetPatterns.values())); |
Without this, two different @modal slots in sibling subtrees both using (...) to intercept the same root-level route would still trigger a false positive from validateRoutePatterns.
| const builder = await createBuilder({ | ||
| root, | ||
| configFile: false, | ||
| plugins: [vinext({ appDir: root })], |
There was a problem hiding this comment.
Passing appDir: root means appDir is the project root, not the app/ subdirectory. Is this intentional? I'd expect appDir: path.join(root, 'app') based on how other tests configure vinext — unless vinext() defaults to <root>/app when appDir is the project root.
Worth double-checking this doesn't accidentally work by falling through to a default.
| import { afterEach, describe, expect, it } from "vite-plus/test"; | ||
| import vinext from "../packages/vinext/src/index.js"; | ||
|
|
||
| const tmpDirs: string[] = []; |
There was a problem hiding this comment.
Nit: The tmpDirs array + afterEach cleanup pattern works, but the withTempDir helper from routing.test.ts is cleaner (auto-cleans in finally). Consider extracting it to a shared test utility if you want to reuse it, or just use the same pattern inline here. Not a blocker.
tests/routing.test.ts
Outdated
| }); | ||
| }); | ||
|
|
||
| it("rejects a single intercepting page resolving to multiple target patterns", () => { |
There was a problem hiding this comment.
Good test — this directly validates the new collectInterceptTargetPatterns function with a synthetic conflict. The test constructs a scenario where the same pagePath maps to two different targetPattern values and verifies it throws.
One thing to consider: it would also be valuable to test the inverse — two different pagePath values that map to the same targetPattern — to document the current behavior (which would currently throw, as discussed in my comment on the implementation).
| was intercepted by the parallel slot. | ||
| </p> | ||
| <div className="font-mono text-sm text-cyan-300"> | ||
| ${product.price.toFixed(2)} |
There was a problem hiding this comment.
Nit: This renders a literal $ followed by the price. In JSX, $ is not interpolated, so this renders correctly as $1.99 etc. — but it reads confusingly like a template literal that's missing backticks. Consider wrapping in braces for clarity:
| ${product.price.toFixed(2)} | |
| {`$${product.price.toFixed(2)}`} |
| Loading this URL directly should render the standalone page. Navigating | ||
| from the gallery should keep the gallery visible and render this content | ||
| in the parallel modal slot instead. | ||
| </p> |
There was a problem hiding this comment.
Same nit as the modal page — this JSX $ works but reads like a broken template literal:
| </p> | |
| {`$${product.price.toFixed(2)}`} |
|
Review posted successfully. Here's a summary of the feedback I provided: Overall assessment: The fix is correct and well-motivated. The root cause (inherited slots causing the same physical intercepting page to appear multiple times) is accurately diagnosed, and de-duplicating by Key feedback points:
|
|
/bigbonk review |
There was a problem hiding this comment.
Good bug fix. The root cause (inherited slots carrying interceptingRoutes to child routes, causing duplicates in validation) is correctly diagnosed and the fix is minimal. A few notes below.
The PR is marked as draft while the author investigates cross-subtree interception semantics (e.g. app/feed/@modal/(..)photo/[id] + app/profile/@modal/(..)photo/[id] both targeting /photo/:id). That's the right call — the Set de-dupe on targetPattern silently allows that case, and it's worth confirming Next.js behavior before merging.
| cachedPageExtensionsKey = null; | ||
| } | ||
|
|
||
| export function collectInterceptTargetPatterns(routes: readonly AppRoute[]): string[] { |
There was a problem hiding this comment.
Exporting this is good — it makes the de-duplication logic directly unit-testable (which the new test exercises). One thing to consider: the function name says "collect" but it also de-duplicates. A name like collectUniqueInterceptTargetPatterns would make the contract clearer, though this is a minor nit.
| const builder = await createBuilder({ | ||
| root, | ||
| configFile: false, | ||
| plugins: [vinext({ appDir: root })], |
There was a problem hiding this comment.
The previous bonk review flagged this as potentially wrong (appDir: root instead of path.join(root, 'app')), but this is actually correct. The appDir option is used as baseDir in the plugin, and path.join(baseDir, 'app') is derived internally (see index.ts:1200). So passing the project root is the intended usage.
However, I'd argue this test would be clearer without the appDir option at all — since the fixture has app/ at the root, the auto-detection logic (index.ts:1185-1196) would find it. Removing appDir: root would test the more common configuration path and avoid this confusion.
| import { afterEach, describe, expect, it } from "vite-plus/test"; | ||
| import vinext from "../packages/vinext/src/index.js"; | ||
|
|
||
| const tmpDirs: string[] = []; |
There was a problem hiding this comment.
The routing.test.ts file has a withTempDir helper that handles cleanup in a finally block, which is safer — if the test throws before tmpDirs.push(root), cleanup never happens. Consider using the same pattern:
async function withTempDir<T>(prefix: string, fn: (dir: string) => Promise<T>): Promise<T> {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix));
try {
return await fn(dir);
} finally {
fs.rmSync(dir, { recursive: true, force: true });
}
}Or better yet, extract the one from routing.test.ts into a shared test utility.
| }, | ||
| ]; | ||
|
|
||
| expect(collectInterceptTargetPatterns([firstRoute, secondRoute])).toEqual(["/photo/:id"]); |
There was a problem hiding this comment.
This test directly validates that two different physical pages targeting the same targetPattern produce a single entry — good coverage for the Set de-duplication.
Worth noting: this also documents that vinext currently allows two different physical intercepting pages to target the same pattern (the Set silently merges them). If that turns out to differ from Next.js behavior, this test will need updating.
| EMPTY_PAGE, | ||
| ); | ||
|
|
||
| invalidateAppRouteCache(); |
There was a problem hiding this comment.
This is the key regression test — it exercises the exact scenario from the bug report (inherited @modal slot causing the same intercepting page to appear on both /intercepting-routes and /intercepting-routes/photo/:id, which the old code rejected).
The assertions at lines 1122-1130 nicely verify that both routes see the same targetPattern and pagePath, confirming the inheritance behavior.
| @@ -0,0 +1,46 @@ | |||
| 'use cache'; | |||
There was a problem hiding this comment.
Is 'use cache' intentional here? This layout receives modal as a prop (a React node from a parallel slot). Caching a layout that renders dynamic slot content could produce stale modal content or cache key explosions depending on how the cache boundary interacts with RSC streaming.
Other layouts in the playground (e.g. parallel-routes/layout.tsx) don't use 'use cache'. If this was copied from another layout template, consider removing it.
|
Review posted successfully. Here's a summary of the feedback I provided: Overall assessment: Good bug fix. The root cause is correctly diagnosed — inherited parallel slots carry their The PR is correctly marked as draft while the author investigates cross-subtree interception semantics. Key feedback points:
|
Summary
This PR adds an
intercepting-routesexample toexamples/app-router-playgroundand fixes a vinext App Router build bug exposed by that example.The bug was in intercepting route validation during App Router route discovery. When a parallel slot is inherited by child routes, the same intercepting page can appear on multiple discovered routes. vinext was flattening all intercepted target patterns and validating them globally, which caused the same physical intercepting page to be counted multiple times and incorrectly rejected as a duplicate route.
What changed
intercepting-routesdemo toexamples/app-router-playground(.)conventionRoot cause
The failure was not caused by the example itself.
For a structure like:
app/intercepting-routes/photo/[id]/page.tsxapp/intercepting-routes/@modal/(.)photo/[id]/page.tsxvinext correctly discovered both the standalone route and the intercepting route. However, because the
@modalslot is inherited by child routes, the same intercepting page was surfaced more than once during route discovery. Validation then treated those repeated entries as separate route definitions and threw:You cannot have two routes that resolve to the same pathThis PR keeps real conflict detection intact while avoiding false positives from inherited slot reuse.
Tests
Added:
tests/intercepting-routes-build.test.tstests/routing.test.tsRan:
pnpm run checkpnpm test -- tests/routing.test.tspnpm test -- tests/intercepting-routes-build.test.tscd examples/app-router-playground && pnpm run buildNotes:
pnpm testis mostly green, but currently has an unrelatedafterAlltimeout intests/pages-router.test.tspnpm run test:e2eis blocked in this environment by WranglerEPERM/EMFILEissues